Add Middleware handlers to StreamableHttpTransport #218
Conversation
sveneld
requested review from
CodeWithKyrian,
Nyholm and
chr-hertel
as code owners
|
@chr-hertel please take a look |
|
On the first glance, i feel like it makes it more sophisticated, is it needed tho? maybe i'm missing the point here ... |
|
Yep, that’s the idea. #194 will be rewritten on top of this middleware approach, and it also opens the door for community/custom middlewares without patching core. |
|
@chr-hertel #221 is a pull request with oAuth implementation based on middleware. I created a new pull request instead of update previous |
|
@chr-hertel maybe we should go step by step and this pull request will be first part for oauth implementing? |
|
In the beginning I was a bit conflicted about bringing in this extension point, bc it felt like we're just doing it for that one case, but it's fair to assume there might be more - and yes, this would be the first step, auth second. WDYT @Nyholm @CodeWithKyrian @soyuka? (see #221 for more context) |
Nyholm
left a comment
Nyholm
left a comment
There was a problem hiding this comment.
I like the idea.
It will help making the SDK more extendable. I have a few small suggestions I want you to address.
Good work!
Nyholm
left a comment
Nyholm
left a comment
There was a problem hiding this comment.
Thank you. I like this PR.
I would like us to consider the stack trace. If I add 14 middleware, then I go in to StreamableHttpTransport::handleRequest(), would I like to see these 14 middleware + 14 MiddlewareRequestHandler in my stack trace?
(Im thinking out loud)
What are the pros and cons for creating a RequestHandler looking something like
class MiddlewareRequestHandler implements RequestHandlerInterface
{
/**
* @param MiddlewareInterface[] $middleware
*/
public function __construct(
private array $middleware,
private \Closure $application,
) {
}
public function handle(ServerRequestInterface $request): ResponseInterface
{
$middleware = array_shift($this->middleware);
if (null === $middleware) {
return ($this->application)($request);
}
return $middleware->process($request, $this);
}
}With this only "active" middleware is in the stack. And there is no extra RequestHandler.
| }; | ||
| } | ||
|
|
||
| private function createRequestHandler(): RequestHandlerInterface |
There was a problem hiding this comment.
Can you move the 4 lines of logic here to the listen() method?
There was a problem hiding this comment.
in this case i should make createRequestHandler protected. Is it ok?
There was a problem hiding this comment.
Im not sure what you mean.
I suggest to update the listen method like:
public function listen(): ResponseInterface
{
$handler = new MiddlewareRequestHandler(
$this->middleware,
\Closure::fromCallable([$this, 'handleRequest']),
);;
return $this->withCorsHeaders($handler->handle($this->request));
}
sveneld
force-pushed
the
middleware
branch
3 times, most recently
from
a152386 to
00947fb
Compare
Yes, the stack trace will contain 14+ middleware, because they are called one by one.
I updated a pull request and made one commit. |
sveneld
force-pushed
the
middleware
branch
5 times, most recently
from
c3871e8 to
4102d1b
Compare
Nyholm
left a comment
Nyholm
left a comment
There was a problem hiding this comment.
Thank you.
Before we release next version, I think we should move CORS to a middleware (Not in this PR)
Ok. Than i will be wait for a merge of this pull request |
5 participants
Motivation and Context
This pull request gives a possibility to add different middlewares to StreamableHttpTransport for different approach.
How Has This Been Tested?
Unit tests
Breaking Changes
No
Types of changes
Checklist
Additional context